Improve E2E Test Fixtures to be less flaky #2208

JustinGrote · 2024-12-05T02:11:39Z

Rewrite Pses Stdio host to be task-based and interface-based
Replace LoggingStream with DebugOutputStream to log LSP communication to debug console when debugging tests
Fix up Tests that require help by offline updating help in all platforms/CI

~~- [ ] Wire up LSP Client logging to XUnit ITestOutputHelper~~
~~- [ ] Wire up PSES Initial File Logging to debug console~~

JustinGrote · 2024-12-05T07:14:08Z

@andyleejordan @SeeminglyScience Did a lot of cleanup here, DAP tests run much faster and I should have cleaned up the race conditions, they've been completely reliable for me so far. The tests should also read fairly cleaner now as well.

andyleejordan · 2024-12-10T19:28:15Z

@JustinGrote running this through the internal pipeline right now!

andyleejordan · 2024-12-10T19:51:07Z

@JustinGrote running this through the internal pipeline right now!

Sooo actually per 20fa478 we cannot currently run the tests on OneBranch since it's stuck with PowerShell 7.3...so this will have to wait until January 😵

andyleejordan · 2025-02-26T22:11:56Z

Hi @JustinGrote, we can finally run the tests again in ADO. Would you want to get this PR updated? Note that I had to skip the getCommand test, it just hung (@SeeminglyScience's guess was that the ADO PowerShell might have network shares in PSModulePath but yeah it just ran for twenty minutes before I killed it).

JustinGrote · 2025-02-28T00:30:36Z

@andyleejordan rebased and passed locally for me. Lets see if ADO likes it.

JustinGrote · 2025-02-28T00:47:54Z

Looks like it works. I can do the logging fixups as a separate item especially now that the other logging stuff was implemented.

Copilot

PR Overview

This PR improves the stability and clarity of the E2E test fixtures by reworking the PSES process host to use task- and interface-based patterns and by replacing the legacy LoggingStream with DebugOutputStream for logging LSP communication.

Converted the StdioLanguageServerProcessHost and related test fixtures to use updated process management with task completion sources.
Replaced LoggingStream with DebugOutputStream to log stream activity only when a debugger is attached.
Updated references in tests and LSP fixtures to use the new host implementation.

Reviewed Changes

File	Description
test/PowerShellEditorServices.Test.E2E/Hosts/StdioLanguageServerProcessHost.cs	Refactored host start/stop logic to use task-based state tracking.
test/PowerShellEditorServices.Test.E2E/Hosts/PsesStdioLanguageServerProcessHost.cs	Updated host constructor and environment variable references for improved test stability.
test/PowerShellEditorServices.Test.E2E/Hosts/DebugOutputStream.cs	Introduced DebugOutputStream to log read/write operations when debugging tests.
test/PowerShellEditorServices.Test.E2E/DebugAdapterProtocolMessageTests.cs	Updated tests to use the new host and logging methods; removed dependency on LoggingStream.
test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs, LSPTestsFixtures.cs, IDebugAdapterClientExtensions.cs	Updated usage and references from the legacy process host to the new one.
Various Process files	Removed legacy PsesStdioProcess and related files to consolidate host management.
src/PowerShellEditorServices/Server/PsesDebugServer.cs	Minor formatting adjustment in the debug server response initialization.

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

test/PowerShellEditorServices.Test.E2E/DebugAdapterProtocolMessageTests.cs

andyleejordan

Nice, I like this a lot, just some questions. Also as far as ADO goes at this point we just have to YOLO it. I can't easily test a PR against the pipeline that would actually block us from releasing if it broke. 🤷 (Long story, really annoying.)

test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs

andyleejordan · 2025-03-04T19:20:08Z

test/PowerShellEditorServices.Test.E2E/PowerShellEditorServices.Test.E2E.csproj

+
+  <ItemGroup>
+    <!-- Used for Update-Help for some test fixtures -->
+    <Content Include="..\PowerShellEditorServices.Test.Shared\PSHelp\**\*"


Would this fix all the tests that depend on help, and could we remove the build step where we Update-Help for the tests?

Only if the tests use Microsoft.PowerShell.Utility cmdlets for their reference. This is only required for Windows PowerShell since it doesn't ship this help in-box, all pwsh flavors do ship help in-box and so we don't have to do this. Assuming we find/adjust all help-based tests to use Microsoft.PowerShell.Utility, then yes this should fix them in AzDO CI

andyleejordan · 2025-03-04T19:21:33Z

test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs

@@ -1177,7 +1181,7 @@ await PsesLanguageClient
        [SkippableFact]
        public async Task CanSendGetCommentHelpRequestAsync()
        {
-            Skip.If(PsesStdioProcess.RunningInConstrainedLanguageMode && PsesStdioProcess.IsWindowsPowerShell,
+            Skip.If(PsesStdioLanguageServerProcessHost.RunningInConstrainedLanguageMode && PsesStdioLanguageServerProcessHost.IsWindowsPowerShell,
                "Windows PowerShell doesn't trust PSScriptAnalyzer by default so it won't load.");


Hey, is this why people are having problems with the formatter "hanging" when they use Windows PowerShell? Food for thought...

It might be! Maybe I can get some LSP trace logs from them and stick in some telemetry to find out.

andyleejordan · 2025-03-04T19:21:57Z

test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs


-            Assert.Contains("Extracts files from a specified archive", updatedCompletionItem.Documentation.String);
+            Assert.Contains("Gets the current date and time.", updatedCompletionItem.Documentation.String);
        }

        [SkippableFact]
        public async Task CanSendHoverRequestAsync()
        {
            Skip.If(IsLinux, "This depends on the help system, which is flaky on Linux.");


Do we still need to skip this test now that the help is mocked?

Maybe not? As you said lets YOLO it as a test.

I also think I separately fixed this test in the other branch so there might be a conflict, I'll rebase.

andyleejordan · 2025-03-04T19:22:31Z

test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs

        public async Task CanSendCompletionResolveWithModulePrefixRequestAsync()
        {
            await PsesLanguageClient
                .SendRequest(
                    "evaluate",
                    new EvaluateRequestArguments
                    {
-                        Expression = "Import-Module Microsoft.PowerShell.Archive -Prefix Slow"
+                        Expression = $"Update-Help Microsoft.Powershell.Utility -SourcePath {s_binDir};Import-Module Microsoft.PowerShell.Utility -Prefix Test -Force"


Ahh sweet, ok yeah, I think there's an Update-Help in the build script too...

andyleejordan · 2025-03-04T19:23:36Z

test/PowerShellEditorServices.Test.E2E/Hosts/PsesStdioLanguageServerProcessHost.cs

+        s_binDir, $"pses_test_logs_{Path.GetRandomFileName()}");
+
+    private const string s_logLevel = "Diagnostic";
+    private static readonly string[] s_featureFlags = { "PSReadLine" };


Do we really pass this through in feature flags still? I didn't think we had anything in feature flags.

I just looked and I didn't see this feature flag actually referenced anywhere, so it can probably be removed but I think that should be the scope of another PR. You're probably seeing it here as a "change" because I moved the file to a new folder I belieeve.

It took longer than 1 minute in Github Actions!

JustinGrote · 2025-03-05T23:21:35Z

@andyleejordan the FindSymbolsInDSCFile test failure appears to be because of 0135baa #1984. You updated the IDs of DSC symbols to include a prefix, but the test doesn't check for that new prefix.

PowerShellEditorServices/src/PowerShellEditorServices/Services/Symbols/Visitors/SymbolVisitor.cs

Lines 275 to 276 in de0eea1

    
           "configuration " + name + " { }", 
        
           nameExtent,

I will "fix" this test but please confirm if you're expecting something else. Also, completions allow for a sortText property in the LSP, maybe we should implement this more explicitly if that's the intention for the prefix?

@andyleejordan

Contengent assuming #2208 (comment) is OK from @andyleejordan

@andyleejordan

Contengent assuming #2208 (comment) is OK from @andyleejordan

JustinGrote · 2025-03-06T00:50:01Z

@andyleejordan I came up with a better way to do the "offline" help update in eaeefa1 and centralized it in the build script again.

andyleejordan · 2025-03-06T05:47:29Z

@andyleejordan the FindSymbolsInDSCFile test failure appears to be because of 0135baa #1984. You updated the IDs of DSC symbols to include a prefix, but the test doesn't check for that new prefix.

PowerShellEditorServices/src/PowerShellEditorServices/Services/Symbols/Visitors/SymbolVisitor.cs

Lines 275 to 276 in de0eea1

"configuration " + name + " { }",

nameExtent,

I will "fix" this test but please confirm if you're expecting something else. Also, completions allow for a sortText property in the LSP, maybe we should implement this more explicitly if that's the intention for the prefix?

Oops. I don't think I'm expecting something else. And if I remember correctly on the sortText, VS Code completely ignored it and applies their own sorting.

…ifact size.

JustinGrote · 2025-03-06T21:19:20Z

@andyleejordan this should be ready for re-review and an internal pipeline test. Changes summary:

Fixed help-based tests to all use Microsoft.PowerShell.Archive, because it has a small help file to be vendored into the codebase
Added update help build script for all pwsh platforms with verify check.
Added upterm to the CI process pre-build. This allows, if you explicitly re-run a job with debug build, to be able to SSH into the runner to interactively troubleshoot/debug issues. Only runs on debug re-runs
Most tests conditional on platforms fixed or mitigated.

JustinGrote self-assigned this Dec 5, 2024

JustinGrote changed the title ~~Initial Pass that implements DebugAdapterServerTestBase~~ Improve E2E Test Fixtures to be less flaky Dec 5, 2024

JustinGrote force-pushed the test/DapE2ETestUpdates branch 3 times, most recently from 5cb7349 to 1d8a574 Compare December 5, 2024 06:47

JustinGrote force-pushed the test/DapE2ETestUpdates branch from 7f0083c to 7ecdd5a Compare December 6, 2024 17:46

JustinGrote force-pushed the test/DapE2ETestUpdates branch from 7ecdd5a to b1bafa3 Compare December 20, 2024 19:32

JustinGrote force-pushed the test/DapE2ETestUpdates branch from b1bafa3 to b5c00a3 Compare February 28, 2025 00:26

JustinGrote marked this pull request as ready for review February 28, 2025 00:48

JustinGrote requested review from Copilot and andyleejordan February 28, 2025 00:48

Copilot AI reviewed Feb 28, 2025

View reviewed changes

test/PowerShellEditorServices.Test.E2E/DebugAdapterProtocolMessageTests.cs Show resolved Hide resolved

JustinGrote force-pushed the test/DapE2ETestUpdates branch from e12f94b to 5642221 Compare February 28, 2025 00:55

andyleejordan reviewed Mar 4, 2025

View reviewed changes

andyleejordan force-pushed the test/DapE2ETestUpdates branch from 5642221 to 4e68bf6 Compare March 4, 2025 19:28

JustinGrote added 2 commits March 4, 2025 13:52

Improve E2E Test Fixtures to be less flaky

9b05bef

Adjust test skips

52feb6c

JustinGrote force-pushed the test/DapE2ETestUpdates branch from 4e68bf6 to 52feb6c Compare March 4, 2025 21:52

Fix dangling IsLinux reference

a476e9f

JustinGrote force-pushed the test/DapE2ETestUpdates branch from e0e8c1e to a476e9f Compare March 4, 2025 22:13

JustinGrote added 4 commits March 4, 2025 15:46

Kick Hover Test issue can down the road

9f0babf

Make CanSendGetCommandRequestAsync Skip in CI Again

bfd87c3

Remove unneded using namespace

66346ce

Bump GetCommandRequest to 2 minutes

c8f1c3e

It took longer than 1 minute in Github Actions!

JustinGrote marked this pull request as draft March 5, 2025 03:03

Update POST

de0eea1

JustinGrote added a commit that referenced this pull request Mar 5, 2025

Fix DSC Test

69ef1bd

Contengent assuming #2208 (comment) is OK from @andyleejordan

Fix DSC Test

2d7d3e2

Contengent assuming #2208 (comment) is OK from @andyleejordan

JustinGrote force-pushed the test/DapE2ETestUpdates branch from 69ef1bd to 2d7d3e2 Compare March 5, 2025 23:42

Fix Help Update Process

eaeefa1

Add upterm debugging to pipeline

4a5a892

JustinGrote force-pushed the test/DapE2ETestUpdates branch 5 times, most recently from 22993ea to 34347ba Compare March 6, 2025 03:01

Expand help update to all potential pwsh versions

b22510a

JustinGrote force-pushed the test/DapE2ETestUpdates branch from 34347ba to b22510a Compare March 6, 2025 03:07

Add PS7 Help

cd20348

JustinGrote added 3 commits March 5, 2025 23:30

Try Help CAB again

36a8998

Apparently it's a zip not a cab on Linux...

1cca650

Fixup PS5.1 Help Script

5a46259

JustinGrote force-pushed the test/DapE2ETestUpdates branch 2 times, most recently from 3eb0adb to 50a806f Compare March 6, 2025 20:41

JustinGrote mentioned this pull request Mar 6, 2025

Test PowerShell Preview in CI instead of Daily #2225

Open

Switch to Microsoft.PowerShell.Archive for help tests for smaller art…

2a65e3d

…ifact size.

JustinGrote force-pushed the test/DapE2ETestUpdates branch from 50a806f to 2a65e3d Compare March 6, 2025 20:58

JustinGrote marked this pull request as ready for review March 6, 2025 21:12

JustinGrote requested a review from a team as a code owner March 6, 2025 21:12

JustinGrote requested a review from andyleejordan March 6, 2025 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve E2E Test Fixtures to be less flaky #2208

Improve E2E Test Fixtures to be less flaky #2208

JustinGrote commented Dec 5, 2024 •

edited

Loading

JustinGrote commented Dec 5, 2024

andyleejordan commented Dec 10, 2024

andyleejordan commented Dec 10, 2024

andyleejordan commented Feb 26, 2025

JustinGrote commented Feb 28, 2025

JustinGrote commented Feb 28, 2025

Copilot AI left a comment

andyleejordan left a comment

andyleejordan Mar 4, 2025

JustinGrote Mar 4, 2025

andyleejordan Mar 4, 2025

JustinGrote Mar 4, 2025

andyleejordan Mar 4, 2025

JustinGrote Mar 4, 2025

JustinGrote Mar 4, 2025

andyleejordan Mar 4, 2025

andyleejordan Mar 4, 2025

JustinGrote Mar 4, 2025 •

edited

Loading

JustinGrote commented Mar 5, 2025

JustinGrote commented Mar 6, 2025

andyleejordan commented Mar 6, 2025

JustinGrote commented Mar 6, 2025 •

edited

Loading

Improve E2E Test Fixtures to be less flaky #2208

Are you sure you want to change the base?

Improve E2E Test Fixtures to be less flaky #2208

Conversation

JustinGrote commented Dec 5, 2024 • edited Loading

JustinGrote commented Dec 5, 2024

andyleejordan commented Dec 10, 2024

andyleejordan commented Dec 10, 2024

andyleejordan commented Feb 26, 2025

JustinGrote commented Feb 28, 2025

JustinGrote commented Feb 28, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

andyleejordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinGrote Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

JustinGrote commented Mar 5, 2025

JustinGrote commented Mar 6, 2025

andyleejordan commented Mar 6, 2025

JustinGrote commented Mar 6, 2025 • edited Loading

JustinGrote commented Dec 5, 2024 •

edited

Loading

JustinGrote Mar 4, 2025 •

edited

Loading

JustinGrote commented Mar 6, 2025 •

edited

Loading